Reviewed 12 of 12 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @SteveLauC)

examples/raft-kv-memstore-network-v2/src/store.rs line 196 at r2 (raw file):
According to the documentation of save_snapshot:

/// This method is called when a snapshot has a higher last-log-id than the current one but
/// smaller than the last-applied-log-id of the state machine.

Why this comparison (also elsewhere)? Or did I misunderstand something?

Code quote:

        // Only save it if the new snapshot contains more recent data than the current snapshot.

        let current_last = current.as_ref().and_then(|s| s.meta.last_log_id);
        if new_snapshot.meta.last_log_id <= current_last {

openraft/src/storage/v2/raft_state_machine.rs line 123 at r1 (raw file):

    /// smaller than the last-applied-log-id of the state machine. In this case, the state machine
    /// only needs to persist the provided new snapshot, without replacing the state machine's
    /// current state(without calling [`Self::install_snapshot`]).

But why would we then bother with saving the snapshot in the first place? The current snapshot is a newer one, right?

Code quote:

    /// This method is called when a snapshot has a higher last-log-id than the current one but
    /// smaller than the last-applied-log-id of the state machine. In this case, the state machine
    /// only needs to persist the provided new snapshot, without replacing the state machine's
    /// current state(without calling [`Self::install_snapshot`]).

openraft/src/storage/v2/raft_state_machine.rs line 133 at r1 (raw file):

    /// When a snapshot needs to be saved but is not newer than the state machine (e.g., when a
    /// leader replicates an already built snapshot to a follower with an up-to-date state machine):
    /// - Only this method will be called

As noted above, why do we need to call it at all?

openraft/src/storage/v2/raft_state_machine.rs line 138 at r1 (raw file):

    ///
    /// Before this method is added, [`Self::install_snapshot`] is responsible to replace the
    /// state machine and save the snapshot.

Suggestion:

    /// Before this method was added, [`Self::install_snapshot`] was responsible to replace the
    /// state machine and to save the snapshot.

openraft/src/core/sm/worker.rs line 133 at r1 (raw file):

                    );

                    self.state_machine.save_snapshot(&snapshot).await?;

As on now, I see there is a fixed order save + install here (or according to the docs, save only, which seems not to be yet implemented). I suppose, this will be kept like this also in the future?

My problem is that in our project, we cannot easily have multiple active snapshots in our state machine, but this effectively requires implementing it. Well, we'll have to solve it somehow...

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @schreter and @SteveLauC)

examples/raft-kv-memstore-network-v2/src/store.rs line 196 at r2 (raw file):

Sorry for lacking of docs about this part:

Because currently there are other thread updating the current snapshot: such as build_snapshot(), which is currently designed to run in another thread and update current snapshot inside it when finished.

In the next PR I'll make build_snapshot() returns the snapshot it builds and pass it to RaftCore to serialize all of the save-snapshot action. Then this order control can be removed.

openraft/src/core/sm/worker.rs line 133 at r1 (raw file):

Right, in future when a snapshot is replicated from leader to follower, the follower is allowed to just save without installing.

I did not follow... it's good that you can have more than one snapshots. Then what's the problem?

openraft/src/storage/v2/raft_state_machine.rs line 123 at r1 (raw file):

It is called to replace the current snapshot with a newer one, which could be one sent by the Leader to this node, a follower.

The current one could be an older one.

openraft/src/storage/v2/raft_state_machine.rs line 133 at r1 (raw file):

This method is called if the input argument snapshot is newer than the current one and smaller than the state in the state machine.

openraft/src/storage/v2/raft_state_machine.rs line 138 at r1 (raw file):

    ///
    /// Before this method is added, [`Self::install_snapshot`] is responsible to replace the
    /// state machine and save the snapshot.

It's so nice of you :)

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @drmingdrmer and @SteveLauC)

openraft/src/core/sm/worker.rs line 133 at r1 (raw file):

No, currently we can't save/install multiple independent snapshots in our SM. We can take multiple snapshots and keep them for arbitrary period of time, but we can only replace the SM as a whole from an external snapshot. That's the problem. Implementing saving multiple snapshots and activating one of them is in principle possible, but not easy.

openraft/src/storage/v2/raft_state_machine.rs line 123 at r1 (raw file):

Yes, but I still don't get it.

We have now install_snapshot and save_snapshot. The documentation here says, snapshot saved here can be retrieved with get_current_snapshot(). But that means, it is the active snapshot of the SM and was installed into the SM.

If not, then get_current_snapshot() should return the last built or last installed snapshot (via install_snapshot), right?

So somehow it's a bit inconsistent/not really well understandable. Maybe the confusion will clear itself when the rest of the work you are planning here is done?

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @schreter and @SteveLauC)

openraft/src/core/sm/worker.rs line 133 at r1 (raw file):

Hmm... In your implementation, the snapshot is considered part of the state machine, correct?

Then it would be also possible to make save_snapshot() optionally implemented: when called, it could do nothing, while install_snapshot() would handle both saving and replacing the snapshot. This approach would simplify the interface for implementations that don't need separate snapshot building functionality.

openraft/src/storage/v2/raft_state_machine.rs line 123 at r1 (raw file):

Yes, get_current_snapshot() returns either the last built or the last installed snapshot.

Without installing or building a new snapshot, get_current_snapshot() should always return the same result.

The snapshot returned from get_current_snapshot() isn't expected to be up-to-date with the current state machine—it represents a historical snapshot. Since you're asking this question, I'm now a bit uncertain about your definition of "active snapshot." To clarify the expected behavior: install_snapshot should replace the "active snapshot," save_snapshot should replace the "active snapshot," and get_current_snapshot() should return the "active snapshot."

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @drmingdrmer and @SteveLauC)

examples/raft-kv-memstore-network-v2/src/store.rs line 196 at r2 (raw file):

Ok, let's see then :-).

openraft/src/core/sm/worker.rs line 133 at r1 (raw file):

In our case, the snapshot is a copy-on-write snapshot of the current state of the SM persistency. While the persistency is running, I can create as many copy-on-write snapshots as I'd like, so it's not a problem to hold multiple snapshots and refer to them for reading them.

However, when trying to overwrite the persistency with a snapshot, I cannot implement copy-on-write - the snapshot overwrites the complete persistency and any other snapshots will be lost. That's the only issue.

I.e., it's not possible to "prepare" a snapshot to be applied later as of now. However, we are looking into ways how it could be possible (it would create a second persistency in save_snapshot(), which then can later replace the existing persistency in install_snapshot() - and invalidate any older snapshots).

openraft/src/storage/v2/raft_state_machine.rs line 123 at r1 (raw file):

As mentioned above, keeping multiple snapshots is not a problem, as long as they are created internally on the existing state machine and not streamed from the outside.